Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: route options are not merged (inner filter overrides outer filter) #7981

Merged
merged 4 commits into from
Oct 14, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Sep 26, 2023

Description
Fixes #7963

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.5 labels Sep 26, 2023
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to imagine scenarios in which this could have been intentionally used, and frankly, I can't. But I'm sure there will be devs who take advantage of current behavior.

Comment on lines +371 to +400
public function testGroupNestedWithOuterAndInnerOption(): void
{
$routes = $this->getCollector();

$routes->group(
'admin',
['filter' => ['csrf']],
static function ($routes) {
$routes->get('dashboard', static function () {
});

$routes->group(
'profile',
['filter' => ['honeypot']],
static function ($routes) {
$routes->get('/', static function () {
});
}
);
}
);

$expected = [
'admin/dashboard' => [
'filter' => ['csrf'],
],
'admin/profile' => [
'filter' => ['honeypot'],
],
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still confusing to me but I guess it's because we can have only one filter when Config\Feature::multipleFilters is disabled.

Will this gonna end up as:

$expected = [
    'admin/dashboard' => [
        'filter' => ['csrf'],
    ],
    'admin/profile' => [
        'filter' => ['csrf', 'honeypot'],
    ],
];

when multipleFilters option will be enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. it is just because using array_merge().
https://3v4l.org/tskmm

It is the current behavior that filters are not merged, and also the reason why the Note in the user guide was added.

One possible behavior would be to merge filters as well, but then it would not be possible to delete filters in inner group. Multiple filters are already the default in 4.5.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about removing filters.

I don't know... now I'm leaning towards staying with the current behavior and not inheriting anything. Because the new behavior seems even more confusing - it's inherited only if there are no other filters. I can imagine people adding a new filter over time and "losing" functionality.

Thanks to the current note, at least we have clear rules to follow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is not that straightforward either.
The current behavior is that if you don't pass an options array to the inner group, the outer options are inherited.

That's why when you set outer filter and add namespace option to inner group, the filter that set in outer suddenly will be gone for the inner route.

It seems that this issue is not so easy to conclude.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a bit more complex than I thought at first. Especially if we add namespaces to it.

Personally, I would probably merge all the declared filters from the external to the internal group using array_merge_recursive and introduce something like filterExclude to remove the filters if needed. However, this seems a bit too complex... not sure if we want to make such big changes.

Although the rules would be clear and need no further explanation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very sorry for my translation.

Yes, to exclude the outside filter, I gave an example of except in the Config. Or we need to write such a filter for all routes except unnecessary ones (this is your option)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with the new feature to exclude route filters in the Config\Filter.
The Config\Filter and route filters are the same filters, but I think mixing these settings is a source of confusion.

Also, I don't like to add a new key for excluding filters in the route option.
It also is complicated.

Copy link
Contributor

@neznaika0 neznaika0 Oct 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this does not cancel your method - we leave the outside filters empty and configure each route individually

I'm sorry, but what you don't like about the exception in the Config is a function of the framework. There is no other way to fine tune

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config\Filter defines filters for URI paths, not for routes.
except excepts the filter to the URI path in Config\Filter.
https://codeigniter4.github.io/CodeIgniter4/incoming/filters.html#except-for-a-few-uris

Filters for a route are different things. If you specify a filter to a route, it is always applied.
The definitions in Config\Filter does nothing to route filters.

What I don't like is to mix the two kind of filters. That is, except in Config\Filter excepts route filters, too.
It is really complicated and confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalsn @neznaika0 @MGatner I changed my mind. I sent another PR #8033 that behaves like michalsn expects in #7981 (comment)

@github-actions github-actions bot added the stale Pull requests with conflicts label Sep 28, 2023
@github-actions
Copy link

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the fix-route-option-merge branch from b2d49e9 to f4013c4 Compare September 29, 2023 05:22
@kenjis kenjis removed the stale Pull requests with conflicts label Sep 29, 2023
@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 1, 2023
@github-actions
Copy link

github-actions bot commented Oct 1, 2023

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the fix-route-option-merge branch from f4013c4 to 9cad762 Compare October 3, 2023 05:43
@kenjis
Copy link
Member Author

kenjis commented Oct 3, 2023

Rebased to resolve a conflict.

@kenjis kenjis force-pushed the fix-route-option-merge branch from 9cad762 to 0fd10e7 Compare October 3, 2023 06:15
@kenjis kenjis removed the stale Pull requests with conflicts label Oct 3, 2023
@github-actions github-actions bot added the stale Pull requests with conflicts label Oct 5, 2023
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis kenjis force-pushed the fix-route-option-merge branch from 0fd10e7 to c8cb5cc Compare October 5, 2023 00:46
@kenjis
Copy link
Member Author

kenjis commented Oct 5, 2023

Rebased to resolve a conflict.

@kenjis kenjis removed the stale Pull requests with conflicts label Oct 5, 2023
@kenjis kenjis marked this pull request as draft October 12, 2023 00:14
@kenjis kenjis changed the title fix: route options are not merged fix: route options are not merged (inner filter overrides the outer filter) Oct 12, 2023
@kenjis kenjis changed the title fix: route options are not merged (inner filter overrides the outer filter) fix: route options are not merged (inner filter overrides outer filter) Oct 12, 2023
@kenjis kenjis merged commit 8d62a57 into codeigniter4:4.5 Oct 14, 2023
53 checks passed
@kenjis kenjis deleted the fix-route-option-merge branch October 14, 2023 21:59
@kenjis
Copy link
Member Author

kenjis commented Oct 14, 2023

Closed by #8033

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.5 breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants